Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add ConfigureHtmlRenderer to MarkdownPipelineBuilder #802

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

Add ConfigureHtmlRenderer to MarkdownPipelineBuilder #802

wants to merge 13 commits into from
+310 −58

Conversation

yngvedh
Copy link

@yngvedh yngvedh commented Jun 9, 2024 •
edited
Loading

I needed to set up image url rewriting in a project of mine and found it a bit tedious not having a way to set it up in the pipeline. So I added the ability to configure the HtmlRenderer from the pipeline builder. As an added bonus, this works with the renderer cache as well.

I did not add any tests but updated the BaseUrl test to configure the html renderer using the pipeline builder.
I did not update changelog.md since it already has been neglected for some time.
I have not spent any time figuring out whether any of the documentation should be updated, please advise.
I could not find any existing PR or issue on this exact issue so I took the liberty to raise this draft PR.

UPDATE:
I added a bare bones builder for HtmlRenderer letting me set the one field I need as a POC. It is basically a factory for TextRendererBase which I also refactored the cache mechanism to use so that too should work for any text renderer. This goes for the ToHtml method since it uses the caching mechanism to get hold of its renderer.

@xoofx
Copy link
Owner

xoofx commented Jun 10, 2024

Thanks. It looks ok at a quick glance.

@yngvedh
Copy link
Author

yngvedh commented Jun 13, 2024

Great, I'll see if I can find the time to flesh it out with more properties and renderers.

@yngvedh
Copy link
Author

yngvedh commented Jun 13, 2024

So I added builders for all kinds of renderers along with all fields. I also added doc comments and updated some tests.

Now, I'm not 100% sure all those fields should be available through the builder. In particular the EnableHtmlFor* options because they seem to only be used internally by the different renderers/extensions. Please let me know whether I should drop some of them.

I'm also not 100% sure all those renderers should be available through the builder at either so let me know if any of them shouldn't and I'll have them removed.

@MihaZupan
Copy link
Collaborator

A couple observations:

  • HtmlRenderer is by far the most used thing. I think it'd be fine to scope any such helpers to just that.
  • I find it odd that we'd effectively duplicate all properties on renderers with fluent extension methods.
    • Could we expose a single ConfigureHtmlRenderer(this builder, Action<HtmlRenderer>) instead?
  • Having the configuration only apply to renderers created internally by the cache is odd / inconsistent with other configurations on the pipeline builder.
    • It's likely you could implement this as an extension instead, modifying the renderer as part of the extension's renderer configuration. That'd mean this configuration would also apply to custom renderers created via the pipeline.

@xoofx
Copy link
Owner

xoofx commented Jun 14, 2024

Indeed, seeing more changes coming, I agree the points from @MihaZupan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mazahuacar Mazahuacar Mazahuacar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yngvedh @xoofx @MihaZupan @Mazahuacar